Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pre-fill billing information on PayPal (work in progress) #204

Closed
wants to merge 3 commits into from

Conversation

BenSturmfels
Copy link
Contributor

@BenSturmfels BenSturmfels commented Dec 12, 2018

I've had this patch running in production with Saleor for about a year and thought it might be useful to others. May not yet be suitable to merge.

Also handles blank phone number which would otherwise cause a baffling "400 Bad Request" from PayPal. (removed as not available on BasePayment.

Related to the possibly abandoned #132.

@PetrDlouhy
Copy link
Contributor

@BenSturmfels Does this get the billing info from Payment.order? That field is not present in the generic implementation of BasePayment, right? So this at least should check if that field is present and if no, it should work without it.

Anyway I had opposite problem - I needed to get rid of the shipping info entirely. Here is the code: PetrDlouhy@360fbde
Maybe it could be part of same logic as setting the shipping info.

Sends the BasePayment billing details to PayPal to avoid the customer having to re-enter the information they've already provided.
@codecov
Copy link

codecov bot commented Oct 5, 2021

Codecov Report

Merging #204 (023c744) into master (eaff94a) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #204      +/-   ##
==========================================
+ Coverage   79.92%   79.93%   +0.01%     
==========================================
  Files          28       28              
  Lines        1783     1784       +1     
  Branches      212      212              
==========================================
+ Hits         1425     1426       +1     
  Misses        251      251              
  Partials      107      107              
Impacted Files Coverage Δ
payments/paypal/__init__.py 84.14% <100.00%> (+0.06%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2b68871...023c744. Read the comment docs.

@BenSturmfels
Copy link
Contributor Author

I've now updated this patch to use the billing information on BasePayment. I'm no longer sending the phone number, because we don't have that information on BasePayment alone (only on Saleor's Payment subclass). I've updated the PayPal tests to include sample billing data.

@WhyNotHugo
Copy link
Member

WhyNotHugo commented Oct 5, 2021 via email

@BenSturmfels
Copy link
Contributor Author

The idea behind this is pretty clear, and the implementation looks good. I'm not using paypal right now, so I'm just wondering: are the payment fields (e.g.: payment.billing_address_1) mandatory? Or is it possible that it's None for some setups?

I'll do some testing and see how PayPal behaves when sending blanks/nulls. We may need to either need to cast to strings or conditionally not pass the address if it's not complete.

I've rebased on master and deployed to production but found that #139 seems to completely break PayPal payments. :(

@PetrDlouhy
Copy link
Contributor

I have tried this code, but I run into serveral PayPal errors when I had something misconfigured in the billing info:

'field': 'transactions[0].item_list.shipping_address',
'issue': 'The specified country requires a postal code'

or

'The combination of country and city is invalid'

or

'field': 'transactions[0].item_list.shipping_address.line1',
'issue': 'Must not be blank'

Since the billing info is usually filled in by user, it makes it very brittle and several validations of user data are needed to make this work right.

Also I don't think, that the information in your comment:

# "shipping_address", the PayPal payment form labels it as "Billing
# address", hence we're passing the billing address.

is correct.
I have labeled the billing info as Ship to in my form:
Snímek obrazovky_2021-10-07_11-03-52

In my project, we don't need any of this, since we are selling subscribtion for 3D models with no physical shipping.
I have solved this problem by setting:

          'application_context': {
               'shipping_preference': 'NO_SHIPPING',
           },

in the payment data.
This disables the shipping address altogether and PayPal doesn't bother users with it at all.

@BenSturmfels Can you please add some setting to the provider, that would set the shipping_preference parameter and doesn't send shipping_address?

@BenSturmfels
Copy link
Contributor Author

I'm going to close this PR because we're no longer maintaining the system that used this patch.

@PetrDlouhy
Copy link
Contributor

@BenSturmfels Which system is no longer maintained? The PayPal payments?
Does that mean, that PayPal will be placed out of django-payments in the future?
I am still using that, and willing to at least fix the problems if something occur.

Also @WhyNotHugo wanted to make table of backend maintenance statuses in #272. Is there some documentation regarding that?

@BenSturmfels
Copy link
Contributor Author

@PetrDlouhy don't panic! My comments only relate to this specific patch, not django-payments or the PayPal integration as a whole.

We previously used this custom patch to django-payments in an online store based on Saleor. That online store is no longer running, so I won't be able to justify the time to further the patch to a point that it could be included in django-payments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants